Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Windows 10X and Two-pane view support #3768

Closed
wants to merge 2 commits into from
Closed

Windows 10X and Two-pane view support #3768

wants to merge 2 commits into from

Conversation

COM8
Copy link
Contributor

@COM8 COM8 commented Feb 19, 2021

Fixes #3173

PR Type

What kind of change does this PR introduce?

  • Feature

What is the current behavior?

The ListDetailsView does not take advantage of multiple screens on Windows 10X devices.

What is the new behavior?

The new and refactored ListDetailsView control is aware of multiple screens when used on Windows 10X devices.
We archive this by replacing the current grid-based layout with the new Two-pane view based layout.

Additional changes

  • I added additional properties: DetailsPaneBackground, DetailsContentTemplateSelector, ListPaneNoItemsContent, ListPaneNoItemsContentTemplate and ListPaneItemTemplateSelector.
  • I added a new NuGet dependency: Microsoft.UI.Xaml to Microsoft.Toolkit.Uwp.UI.Controls.Layout, so I would be able to use the TwoPaneView class regardlessof the current Windows 10 version.
  • Renamed the ListCommandBar property to ListPaneCommandBar. For more consitency with property names.
  • Renamed the DetailsCommandBar property to DetailsPaneCommandBar. For more consitency with property names.

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tested code with current supported SDKs
  • Pull Request has been submitted to the documentation repository instructions. Link: #559
  • Sample in sample app has been added / updated (for bug fixes / features)
  • New major technical changes in the toolkit have or will be added to the Wiki e.g. build changes, source generators, testing infrastructure, sample creation changes, etc...
  • Tests for the changes have been added (for bug fixes / features) (if applicable)
  • Header has been added to all new source files (run build/UpdateHeaders.bat)
  • Contains NO breaking changes

Other information

Old behavior

Old Toolkit

New behavior

New Toolkit

@ghost
Copy link

ghost commented Feb 19, 2021

Thanks COM8 for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌

@COM8
Copy link
Contributor Author

COM8 commented Feb 19, 2021

What should I do about the documentation over at MicrosoftDocs/WindowsCommunityToolkitDocs#304?
Should I create a new file called ListDetailsView in the appropriate path since I noticed it still uses the MasterDetailsView documentation.

@michael-hawker
Copy link
Member

@COM8 yeah, we haven't updated the documentation with the rename yet. We'll be doing that in the next couple of weeks as we prep for release. I can let you know when that happens and we can rebase that PR of yours after?

@michael-hawker michael-hawker added this to the 7.1 milestone Feb 19, 2021
@michael-hawker michael-hawker added WinUI 💠 Related to WinUI 3 Version or when paired with External can mean requires fix in WinUI 2/3. controls 🎛️ labels Feb 19, 2021
@COM8
Copy link
Contributor Author

COM8 commented Feb 20, 2021

@COM8 yeah, we haven't updated the documentation with the rename yet. We'll be doing that in the next couple of weeks as we prep for release. I can let you know when that happens and we can rebase that PR of yours after?

Sounds great.

@michael-hawker
Copy link
Member

@COM8 looks like there's a build issue?

D:\a\1\s\UnitTests\UnitTests.UWP\UI\Controls\Test_ListDetailsView.cs(22,49): error CS1061: 'ListDetailsView' does not contain a definition for 'SelectedIndex' and no accessible extension method 'SelectedIndex' accepting a first argument of type 'ListDetailsView' could be found (are you missing a using directive or an assembly reference?) [D:\a\1\s\UnitTests\UnitTests.UWP\UnitTests.UWP.csproj]
         D:\a\1\s\UnitTests\UnitTests.UWP\UI\Controls\Test_ListDetailsView.cs(42,29): error CS1061: 'ListDetailsView' does not contain a definition for 'SelectedIndex' and no accessible extension method 'SelectedIndex' accepting a first argument of type 'ListDetailsView' could be found (are you missing a using directive or an assembly reference?) [D:\a\1\s\UnitTests\UnitTests.UWP\UnitTests.UWP.csproj]
         D:\a\1\s\UnitTests\UnitTests.UWP\UI\Controls\Test_ListDetailsView.cs(53,29): error CS1061: 'ListDetailsView' does not contain a definition for 'SelectedIndex' and no accessible extension method 'SelectedIndex' accepting a first argument of type 'ListDetailsView' could be found (are you missing a using directive or an assembly reference?) [D:\a\1\s\UnitTests\UnitTests.UWP\UnitTests.UWP.csproj]
         D:\a\1\s\UnitTests\UnitTests.UWP\UI\Controls\Test_ListDetailsView.cs(54,29): error CS1061: 'ListDetailsView' does not contain a definition for 'SelectedIndex' and no accessible extension method 'SelectedIndex' accepting a first argument of type 'ListDetailsView' could be found (are you missing a using directive or an assembly reference?) [D:\a\1\s\UnitTests\UnitTests.UWP\UnitTests.UWP.csproj]
         D:\a\1\s\UnitTests\UnitTests.UWP\UI\Controls\Test_ListDetailsView.cs(67,49): error CS1061: 'ListDetailsView' does not contain a definition for 'SelectedIndex' and no accessible extension method 'SelectedIndex' accepting a first argument of type 'ListDetailsView' could be found (are you missing a using directive or an assembly reference?) [D:\a\1\s\UnitTests\UnitTests.UWP\UnitTests.UWP.csproj]
         D:\a\1\s\UnitTests\UnitTests.UWP\UI\Controls\Test_ListDetailsView.cs(78,48): error CS1061: 'ListDetailsView' does not contain a definition for 'SelectedIndex' and no accessible extension method 'SelectedIndex' accepting a first argument of type 'ListDetailsView' could be found (are you missing a using directive or an assembly reference?) [D:\a\1\s\UnitTests\UnitTests.UWP\UnitTests.UWP.csproj]

@COM8
Copy link
Contributor Author

COM8 commented Mar 12, 2021

Fixed. Sorry forgot to check the CI run since it took soo long the first time :D

Copy link
Contributor

@skendrot skendrot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm glad this got in here. I had a hard time reviewing the code due to methods getting moved around what appears to be randomly.
I'm concerned that this is slated for 7.1. It contains a lot of breaking changes

/// </summary>
protected override void OnApplyTemplate()
/// <param name="animate">False to skip animations.</param>
private void SetVisualState(bool animate)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Methods have been moved around making review very hard. private methods should be below public/protected methods

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed the method order.

@ghost
Copy link

ghost commented Mar 16, 2021

This PR has been marked as "needs attention 👋" and awaiting a response from the team.

@COM8
Copy link
Contributor Author

COM8 commented Mar 16, 2021

Sorry for the moved arround methods.
That's because for this PR I rebuild the control from the beginning up again and extended it to fit my need in my app.
In case it's really required I can reorder all methods again, but this would take some time.

@michael-hawker michael-hawker mentioned this pull request May 25, 2021
27 tasks
@michael-hawker
Copy link
Member

@COM8 if you'd like to get this in sooner, think we can do things without changing property names or types? Or is that pretty pivotal to the other updates you've made?

We're not too sure when our next major version is going to be. Otherwise, we may have another spot in the next month or so we could put this to get some more folks to test out the changes until such time we can swap them out.

@ghost ghost removed the needs attention 👋 label Jun 17, 2021
@COM8
Copy link
Contributor Author

COM8 commented Jun 26, 2021

Sure, I can change the name of for example ListPaneCommandBar back to ListCommandBar, but this is then not consistent to the other teminology used in the docs. There you are always talking abot list pane and details pane.

@michael-hawker
Copy link
Member

@COM8 yeah, if we want to move forward with the PR though, we wouldn't want to make breaking changes for our upcoming minor release. I think it's something we can still clarify with the documentation comments on the properties.

Did you want to keep them the same so we can move forward? Couple of conflicts to resolve as well?

@COM8
Copy link
Contributor Author

COM8 commented Aug 12, 2021

Ok, I will work on it in the next to weeks so we can move on :)

@COM8
Copy link
Contributor Author

COM8 commented Aug 15, 2021

@michael-hawker Done.
Just need to update the docs.
How do I add properties there? Do I edit the docs manually or can I generate the framework somehow for the new properties?

@XAML-Knight
Copy link
Contributor

Just need to update the docs.
How do I add properties there? Do I edit the docs manually or can I generate the framework somehow for the new properties?

Hi @COM8, for guidance on updating the Docs, take a look here, here, and here.

  • Currently, this is a manual process

@COM8
Copy link
Contributor Author

COM8 commented Aug 23, 2021

Just need to update the docs.
How do I add properties there? Do I edit the docs manually or can I generate the framework somehow for the new properties?

Hi @COM8, for guidance on updating the Docs, take a look here, here, and here.

* Currently, this is a manual process

Thanks. Done: MicrosoftDocs/WindowsCommunityToolkitDocs#559

@michael-hawker michael-hawker added the next preview ✈️ Label for marking what we want to include in the next preview release for developers to try. label Aug 24, 2021
@michael-hawker
Copy link
Member

Thanks @COM8 as long as we're back to no breaking changes, we can remove that label and get this in for 7.1 this week I hope.

@michael-hawker michael-hawker added for-review 📖 To evaluate and validate the Issues or PR and removed sdkcheck 🏁 labels Aug 24, 2021
@XAML-Knight
Copy link
Contributor

Hi @COM8 , it appears you're trying to merge this code in from the master branch of your forked repo.

Please be aware of the rules & stipulations of submitting code into the Toolkit repo:

image

@COM8
Copy link
Contributor Author

COM8 commented Aug 24, 2021

@michael-hawker should I create a new PR from my feature/two-pane-view branch, so the @XAML-Knight is happy?

@@ -162,7 +212,7 @@ public partial class ListDetailsView
nameof(CompactModeThresholdWidth),
typeof(double),
typeof(ListDetailsView),
new PropertyMetadata(720d, OnCompactModeThresholdWidthChanged));
new PropertyMetadata(640d));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did we change the default value here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed this value, so we have a better two too one ratio between the CompactModeThresholdWidth and the ListPaneWidth.

@michael-hawker
Copy link
Member

@michael-hawker should I create a new PR from my feature/two-pane-view branch, so the @XAML-Knight is happy?

Yes, can you submit a new PR from your feature/two-pane-view branch, doesn't look like we can edit the branch this PR is coming from. Thanks!

@michael-hawker
Copy link
Member

Going to close this one, as we'll wait to do final review on the new PR opened.

@COM8
Copy link
Contributor Author

COM8 commented Aug 27, 2021

#4197

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
controls 🎛️ doc-InDraft ⏳ for-review 📖 To evaluate and validate the Issues or PR introduce breaking changes 💥 next preview ✈️ Label for marking what we want to include in the next preview release for developers to try. WinUI 💠 Related to WinUI 3 Version or when paired with External can mean requires fix in WinUI 2/3.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants